Skip to content

Conversation

@DelevoXDG
Copy link
Contributor

@DelevoXDG DelevoXDG commented Nov 24, 2025

@DelevoXDG DelevoXDG force-pushed the zdobnikau/execute-save-outputs branch from ccd2120 to c73263a Compare November 24, 2025 00:54
@DelevoXDG DelevoXDG force-pushed the zdobnikau/execute-save-outputs branch from 41c979a to 037b8ef Compare November 25, 2025 10:22
@DelevoXDG DelevoXDG marked this pull request as ready for review November 25, 2025 12:42
@DelevoXDG DelevoXDG requested a review from a team as a code owner November 25, 2025 12:42
@DelevoXDG DelevoXDG force-pushed the zdobnikau/execute-save-outputs branch from 6b1371c to 0ba8159 Compare December 2, 2025 09:46
@DelevoXDG DelevoXDG changed the base branch from main to zdobnikau/bump-cairo December 2, 2025 09:47
@DelevoXDG DelevoXDG force-pushed the zdobnikau/execute-save-outputs branch from 0ba8159 to a267865 Compare December 2, 2025 10:16
@DelevoXDG DelevoXDG force-pushed the zdobnikau/bump-cairo branch from 5023266 to 035f605 Compare December 2, 2025 13:57
@DelevoXDG DelevoXDG force-pushed the zdobnikau/execute-save-outputs branch from 58546ae to f62beb5 Compare December 2, 2025 13:57
Comment on lines +336 to +347
if args.run.save_program_output
&& let Some(output) = &execution_output
{
let program_output_path = execution_output_dir.join(EXECUTE_PROGRAM_OUTPUT_FILENAME);
fs::write(program_output_path, output.as_str())?;
}

if args.run.save_print_output {
let print_output_path = execution_output_dir.join(EXECUTE_PRINT_OUTPUT_FILENAME);
fs::write(print_output_path, &captured_print_output)?;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder how useful is it, to save this to files, instead of printing to stdout? Can't we just parse the stdout wherever we need to use this? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I based this off of the decision we made in the past regarding scarb prove: Add scarb prove #1900 (comment)

    Although I'm not sure that's super problematic since we have control over the logic of both extensions, I suppose if extracting N from Saving output to [..]N is unstable, then extracting multiline outputs could be considered even less stable 🤔
    That's why we use SCARB_EXECUTION_ID, and it seemed reasonable to me not to rethink this here

  2. Another thing that concerned me - verbosity.

    • If we pass verbosity from doc to execute, and that verbosity is --quiet, the output is lost
    • If we force a normal verbosity for execute, we'd have to capture execute stdout, and then somehow figure out based on doc verbosity what to print and not to print. Since we'd have only raw text at our disposal, there would be no way to filter out messages based on their intended level and doc verbosity
      Maybe there's some elegant solution to this, but I just didn't think it's worth going there since the files should just work 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point, but I don't really think these two are comparable.

For starters, I don't think that using HintProcessor to capture print is reliably what we want. It seems it can be bypassed (see running with bootloader), but also it runs on the assumption that println! it's the only possible way to access stdout (which I am not sure is true).

It shouldn't require any unpredictable parsing, as we can expect the program output to be the last message when run with --program-output flag. We can use json output format in ui, to output it as easily parsable json. We can also do what we do in scarb metadata, that is, run the command with --quite, but set the output to always be printed regardless of verbosity (imho makes sense, if you are explicitly for it to be printed).

@DelevoXDG DelevoXDG requested a review from maciektr December 10, 2025 10:06
@DelevoXDG DelevoXDG force-pushed the zdobnikau/execute-save-outputs branch from f62beb5 to 84d2481 Compare December 18, 2025 13:31
@DelevoXDG DelevoXDG requested a review from Draggu as a code owner December 18, 2025 13:31
@DelevoXDG DelevoXDG changed the base branch from zdobnikau/bump-cairo to main December 18, 2025 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants